-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Events #41
base: master
Are you sure you want to change the base?
Events #41
Conversation
77308b9
to
80d1691
Compare
Works locally with latest chrome, but CI wasn't running with the previous PR, so let's see if this works in all browsers. |
@alvinlindstam Looks like the storage events don't quite behave as expected in IE |
@danielstjules Ah, in what way? I'd be happy to assist in fixing it if possible. |
@alvinlindstam Here's a link to the job associated with your original PR: https://travis-ci.org/zendesk/cross-storage/builds/242063000 The squashed commit is here: 360b896 IE in particular is firing more events than intended. I can probably test against a VM some time next week. |
Ah, I guess it's related to the known issues mentioned on https://caniuse.com/#feat=namevalue-storage.
Should we try to use their suggested workaround or document the bugs? The same issues would occur if using vanilla localStorage on the same host with IE. The workaround would be more difficult to do on the client side however, so if it should be worked around it should probably be on the hub side. But continuously polling and storing all key-value-pairs in localstorage could be quite a performance issue (mainly memory) depending on the stored data. |
Is this feature still planned? Regarding this:
Maybe this could be an opt-in feature so that this cost is not incurred unless you intend to use events? |
Continuation of #32